-
Notifications
You must be signed in to change notification settings - Fork 246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Demonstrate issue where SuperTextFieldInspector.findScrollOffset()
value is incorrect
#2256
base: main
Are you sure you want to change the base?
Demonstrate issue where SuperTextFieldInspector.findScrollOffset()
value is incorrect
#2256
Conversation
…out of sync on mobile
cc/ @matthew-carroll |
SuperTextFieldInspector.findScrollOffset()
value is incorrect
I see that you added a number of expectations. I assume they all fail? What values are each of them reporting? |
@CillianMyles ping on the earlier review questions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the delay in getting back to you on this one!
I added some information around what happens in the tests + values you were looking for.
// Since the scrollable content is taller than the viewport, and since | ||
// the bottom of the text field is aligned with the bottom of the | ||
// viewport, the scroll offset should be greater than 0. | ||
expect(SuperTextFieldInspector.findScrollOffset(), greaterThan(0.0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of the test:
testWidgetsOnMobile("multi-line is vertically scrollable when text spans more lines than maxLines", (tester) async {
And for both Android and iOS, the test fails because the value for SuperTextFieldInspector.findScrollOffset()
is 0
// Issue is not present on desktop, further leading me to think that the | ||
// issue is somehow related to that comment about the scroll offsets | ||
// being out of sync in the mobile scroll view. | ||
expect(SuperTextFieldInspector.findScrollOffset(), greaterThan(0.0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Desktop equivalents pass (so the value is > 0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Printed the actual value here as 20, which makes sense considering we have a scrollable of height 40 with content of height 60
@CillianMyles I ran into this issue while working on #2316, but I'm not sure if it's the same issue you are facing. It seems that sometimes the |
@angelosilvestre I assigned this to you so that you can dig further into the root cause after we decide what to do about restoring the scroll offset. |
As requested in a previous PR: #2139 (comment)
This PR demonstrates that the value of
SuperTextFieldInspector.findScrollOffset()
is incorrect sometimes after vertical scrolling on mobile.I guess maybe this comment might contain a clue:
https://github.com/superlistapp/super_editor/blob/main/super_editor/lib/src/super_textfield/infrastructure/text_scrollview.dart#L523-L537